-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(sftp-card): tsx, styled to scss, removed unused classNames, fixed checkbox padding, fixed cleaning up of the password #95474
Conversation
…fixed checkbox padding, fixed cleaning up of the password
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~303 bytes removed 📉 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the problem, and kudos for taking the extra step to refactor to TS 👍 I think we should consider dropping the use of connect
, though (see my comment about that).
Also, could @matt-west please take a look at the margin between the Enable SSH
toggle and the SSH snippet input and verify that it looks good from a design perspective? I believe this PR restores what we had previously, but it looks slightly cramped to me, so I just want to take the opportunity to tweak that if needed.
type SftpCardComponentProps = SftpCardProps & { | ||
translate: LocalizeProps[ 'translate' ]; | ||
siteId: number | null; | ||
siteSlug: string | null; | ||
currentUserId: number | null; | ||
username: string | null; | ||
password: string | null; | ||
siteHasSftpFeature: boolean; | ||
siteHasSshFeature: boolean; | ||
isSshAccessEnabled: boolean; | ||
isLoadingSftpData: boolean; | ||
createSftpUser: ( siteId: number | null, currentUserId: number | null ) => void; | ||
requestSftpUsers: ( siteId: number | null ) => void; | ||
resetSftpPassword: ( siteId: number | null, sshUsername: string | null ) => void; | ||
requestSshAccess: ( siteId: number | null ) => void; | ||
enableSshAccess: ( siteId: number | null ) => void; | ||
disableSshAccess: ( siteId: number | null ) => void; | ||
removePasswordFromState: ( siteId: number | null, username: string | null ) => void; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I typically avoid connect
when converting JS files to TS is to avoid this redundancy. In other words, useSelector
calls provide us with "free" types, whereas we have to manually respecify types when using connect
.
I would advise we do the same in this case and employ useSelector
and useDispatch
instead of connect. It'll make the code more compact and future refactorings easier.
.sftp-card__ssh-field .sftp-card__input { | ||
margin-top: 5px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, I would move this selector closer to the other .sftp-card__input
selector.
@@ -133,9 +126,19 @@ export const SftpCard = ( { | |||
requestSshAccess( siteId ); | |||
} | |||
} | |||
return onDestroy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does indeed look like a mistake, even if it's hard to tell for sure at first glance. Did you verify this somehow, @nightnei?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as a result - we didn't clean it up during unmounting and it was possible to notice it for a short period of time when we did the second mounting (go back and forth between pages). Why for the short period of time - since we retrieve the list of users every time during mounting, so it wasn't a big issue. But for safety reasons - it's better to clean it up as soon as possible (it was what the author intended to do, but called onDestroy by mistake, and called it with wrong arguments - userId instead of username).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping @fredrikekelund!
I agree, we need some more spacing between the control and snippet below. 16px
should be enough.
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping connect
broke the tests, so I had to make some tweaks to get them working again. LGTM now, though 👍
const { username, password } = useSelector( ( state ) => { | ||
let username; | ||
let password; | ||
|
||
const users = getAtomicHostingSftpUsers( state, siteId ); | ||
|
||
if ( ! disabled && users !== null ) { | ||
if ( users.length ) { | ||
// Pick first user. Rest of users will be handled in next phases. | ||
username = users[ 0 ].username; | ||
password = users[ 0 ].password; | ||
} else { | ||
// No SFTP user created yet. | ||
username = null; | ||
password = null; | ||
} | ||
} | ||
|
||
return { username, password }; | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied the old logic exactly because this component relies on a difference between null
and undefined
values here (something I learned while fixing the tests).
Definitely not an ideal solution to begin with, but the scope of this PR is already pretty large, so if we want to fix that, we should do it in another PR (I don't think it warrants fixing right now btw)
Related to #95435
Proposed Changes
With this PR we are fixing the gap for
Enable SSH access for this site
checkbox.And also I decided to improve a few things:
styled
-css to.scss
, for consistency with most of the component in CalypsoI know that in Calypso we don't force to use tsx, but I advocate everybody to do it. 4-th point is good example, during rewriting it I faced a bug. It wasn't critical, but still.
4.1 We provided 3 arguments instead of 2 - the function expects
siteId, username
, but we providesiteId, currentUserId, username
.4.2 Cleaning up process was incorrect - we invoked
destroy
function immediately during render process, instead of on unmounting. We can see that intention was to return it, but by mistake it was immediately invoked.Testing Instructions
Main issues
Refactor